@willdurand, thanks a lot for getting this started, I agree with you that it looks very likely that we will have to rethink part of the addons-linter (at least the part more closely related to the eslint internals) and choose the strategy that has the higher chances to not be breaking again too easily in the near future.

I'll come back to this asap, I have to look a bit more closely into these changes to be able to provide a more detailed feedback, think about the potential side-effects they may have and what other strategies we may have, if any (you analysis seems pretty convincing, but I wasn't involved in this project when we did the initial research and took the current design choices, and so I want to dig a bit to have some more detailed opinions and be able to provide a better feedback).

@willdurand I spent some time looking a bit more into possible other cons of the current approach drafted in this PR, and I think that the only one that we may want to find a solution for is the fact that this version isn't yet able to collect coverage for the js rules files.

The test for the custom rules defined in this repository are actually being tested, but eslint is loading the webpack bundled modules and jest isn't instrumenting and collecting coverage for them.

Let me know what to you think about the proposed strategy.

Collect coverage data from the custom rules modules

(NOTE: I pushed the changes described below in my fork, in case you want to give it a try quickly by just importing the patch: rpl@3864f80)

I looked into some options to collect that additional coverage data, but the only one that I could think of and that also works as expected without too much work is the following one:

• make the rule modules loadable correctly when "eslint is requiring them during the test case" (which is internally using babel-jest) and when "eslint is requiring them in production" (which will load them from the dist/rules/javascript dir as in this version of the PR), the underlying issue seems to be mainly that babel-jest is returning to eslint require calls a module object that looks like { default: { create: Function } } but eslint expects the modules to be objects that looks like { create: Function }, the simplest way to make a valid es6 module to work in both cases as expected seems to be doing something like the following:

const rule = {
  create(context) {
     ...
  },
}; 
export default rule;
export const { create } = rule;

• in "tests/unit/helpers.js" we could then export a test helper to help us with making sure that the jsScanner is going to use the rule sources (instead of the bundling ones) in the unit tests:

 export function runJsScanner(
   jsScanner,
   { fixtureRules = [], scanOptions } = {}
 ) {
   // global.appRoot is being set in tests/setup.js.
   const ruleSource = path.join(global.appRoot, 'src/rules/javascript');
   const fixturePaths = fixtureRules.map(getJsRulePathForRule);
   return jsScanner.scan({
     ...scanOptions,
     _rulePaths: [ruleSource].concat(fixturePaths),
   });
 }

• Then use the runJsScanner helper in the tests where we want to collect coverage data from the custom rules and where we need to load mock eslint rules.

I did try this locally and looking to the coverage summary it seems that this would be enough to get back a very good chunk of our custom rules test coverage data.

The changes to the rule modules are actually not that different from what you were originally doing (converting them into commonjs modules), but I think that it is actually good that we do not load in production anything that isn't actually coming from the dist directory, and transpiling the eslint rule into their own commonjs modules in dist/rules/javascript sounds still the right approach to me.

Adding a new integration test

I think that it would also good to add one integration test, at least one smoke that that would break if the rules in dist/rules would be missing and eslint fail to load them, e.g. something like the following test case defined in a new test file added in "tests/integration/addons-linter":

   it('linter should pass if ran on a simple valid extension', async () => {
     const fixture = path.resolve(
       __dirname,
       '..',
       '..',
       'fixtures',
       'webextension_es6_module'
     );
     const { exitCode, stderr, stdout } = await executeScript('addons-linter', [
       '-o',
       'json',
       fixture,
     ]);
     expect(stdout).toContain('"summary":{"errors":0,"notices":0,"warnings":0}');
     expect(stderr).toBe('');
     expect(exitCode).toBe(0);
   });

Thanks @willdurand, this looks good to me. I'm wondering about that dotfiles: true config that was used in the previous version and if we do have coverage for that behavior, would you mind to double-check that?

Besides that, follows some small nits related to some tests and the webpack config.

As a side note, it may be good to release this change as part of a major version bump (to be fair I think that we did already discussed briefly about this and we both agreed about that, but I thought to mention it in case I was wrong and we didn't actually talk about it yet).